Fix cursor pagination with optional tie-breaker#2080
Conversation
There was a problem hiding this comment.
Pull request overview
Adds optional composite cursor pagination (primary + secondary “tie-breaker” column) to prevent skipping/duplication when multiple rows share the same primary pagination value (OPS-3810), and wires it into Flow listing.
Changes:
- Extend
Paginatorto optionally encode/decode a secondary cursor field and apply composite cursor filtering. - Extend
buildPaginatoroptions to accept a secondary pagination column. - Add integration tests covering composite pagination behavior when many rows share the same primary timestamp.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/server/api/test/integration/helper/pagination/paginator.integration.test.ts | Adds integration coverage for composite pagination using a secondary ID tie-breaker. |
| packages/server/api/src/app/helper/pagination/paginator.ts | Implements secondary cursor key support and composite cursor filtering + ordering. |
| packages/server/api/src/app/helper/pagination/build-paginator.ts | Exposes secondary pagination configuration via builder options. |
| packages/server/api/src/app/flows/flow/flow.service.ts | Uses the new secondary tie-breaker pagination for flow listing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bigfluffycookie
left a comment
bigfluffycookie
left a comment
There was a problem hiding this comment.
its good that you refactor things, but as a reviewer I now need to figure out which parts are changes for the fix, and which parts you just moved around. Please add comments explaining which parts are refactor and which parts are changes.
| const isCustomColumn = | ||
| this.paginationColumnName && cursors[CUSTOM_PAGINATION_KEY]; | ||
| const columnName = isCustomColumn | ||
| ? this.paginationColumnName | ||
| : `${this.alias}.${PAGINATION_KEY}`; | ||
| const paramName = isCustomColumn ? CUSTOM_PAGINATION_KEY : PAGINATION_KEY; |
There was a problem hiding this comment.
We compute whether the custom pagination key is being used and derive columnName and paramName from that. I moved that logic into resolveCursorContext, and it now also supports resolving the secondary column and parameter name when composite pagination is active.
| if (this.hasBeforeCursor() && !this.hasAfterCursor()) { | ||
| queryString = `${columnName} ${operator} (:${paramName}::timestamp + INTERVAL '1 millisecond')`; | ||
| } else { | ||
| queryString = `${columnName} ${operator} :${paramName}::timestamp`; |
There was a problem hiding this comment.
The query-string construction logic is now split across three helpers:
- applySingleColumnCursorFilter
- applyCompositeCursorFilter
- buildComparisonClause
where the first two decide the shape of the filter, and buildComparisonClause builds the DB comparison fragments they use.
| return order === Order.ASC ? Order.DESC : Order.ASC; | ||
| } | ||
|
|
||
| private buildComparisonClause({ |
There was a problem hiding this comment.
We used to build the DB specific comparison inline for a single pagination key. This logic is now extracted and expanded to support both timestamp and non timestamp fields, because pagination now needs to compare both the primary key and the secondary key.
The main functional addition here is the timestamp = handling with a 1ms window, which is used to treat rows in the same timestamp bucket as equal before applying the secondary key.
| ); | ||
| } | ||
|
|
||
| private applyCompositeCursorFilter( |
There was a problem hiding this comment.
This is the main functional change for the fix. It builds two branches in the WHERE condition: the first branch handles rows that are already beyond the cursor based on the primary key, and the second branch handles rows that have the same primary value as the cursor row and therefore need the secondary key to decide paging order correctly. Earlier only the first branch existed, which is why rows with the same primary value could be skipped.
|
Fixes OPS-3810.
Additional Notes
We create benchmark workflows in bulk, so many rows share the same updated timestamp. With cursor pagination, page boundaries previously filtered using only updated < cursor, which caused rows with the same timestamp to be skipped across pages. This PR adds support for an optional secondary pagination key (used as a secondary sort key) to break ties when primary timestamps are identical.